Fix core generator test nullable and constant-assertion warnings - #7494

@codex review

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

CodeRabbit Verify

• Run CodeRabbit Verify to test the changes in this pull request.

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 8c2550fdf6

Confidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Replaces the constant assertion with a real hook-generator compilation check and adds an explicit failure when NuGet cannot provide a download resource.

• The dashed-assembly test now checks generated hook code.
• The NuGet helper names a missing package download resource.

_{Reviews (1) · Last reviewed commit: "Fix core generator test warnings (#7428)" · Reviewed by Greptile}

Review: looks good, with one optional hardening suggestion.

• NuGetDownloader.cs: the explicit ?? throw replaces a nullable-deref warning with a clear failure message. Correct and minimal.
• AssemblyNamesWithDashesTests.cs: this is a real improvement over Assert.That(true).IsTrue(). It runs HookMetadataGenerator against a compilation named test-assembly, then checks that output exists and that neither the generator diagnostics nor the output compilation have errors.

Suggestion (non-blocking): I couldn't confirm the test fails without the #2919 fix. The test checks 'generated and compiles', but I didn't verify that a dashed name breaks the hook generator's output when the sanitization is absent. I only saw AssemblyName = typeSymbol.ContainingAssembly.Name at HookMetadataGenerator.cs:324. If the regression is only visible in code emitted elsewhere, e.g. a different generator or the aggregate step, this test could pass even with the bug reintroduced. Temporarily reverting the sanitization locally and confirming the test goes red would settle it. Asserting that the generated text contains no - in an identifier position would make the intent more explicit.

Also, the PR dropped the TestsBase base class and the doc comment explaining the #2919 background. Consider keeping a one-line reference to #2919 on the test so its purpose stays discoverable.

Nothing else to flag.

Reviewed all review surfaces for 8c2550f. Codex reports no findings; Claude explicitly reports no blockers; Greptile reports safe to merge. No inline threads exist.

Disposition of Claude's optional suggestions: the replacement test now exercises actual hook generation and checks the resulting compilation for errors with a dashed assembly name, satisfying this warning-cleanup change. Proving sensitivity to the historical #2919 implementation and restoring its background comment are optional further hardening; neither is required for this change, so no additional push is planned. The historical reference remains available in this review and the file history.

Promoting this approved head to ready. CI and the promotion-triggered Claude review must complete before auto-merge can be armed.

Review: looks good.

• NuGetDownloader.cs: the explicit ?? throw removes the nullable warning and gives a clear failure instead of an NRE. Correct.
• AssemblyNamesWithDashesTests.cs: this replaces Assert.That(true) with a real generator run. GeneratorTestInput and ReferencesHelper resolve, since the csproj already links the shared helper, and the TUnit.Tests.Shared namespace matches.

Two non-blocking suggestions:

1. The test asserts there are no compile errors, but it never shows that the dashed assembly name is what the old bug depended on. Check that it fails when the sanitisation in HookMetadataGenerator is reverted. If it doesn't, the Before(HookType.Assembly) hook may not emit the assembly-derived variable, and the test would be as vacuous as before. Asserting that the generated text contains test_assembly and not test-assembly would tie it to the actual regression.
2. output.GetDiagnostics() also reports errors from test-host reference gaps, which could make the test flaky if references change. That is acceptable, but scoping it to errors in generated trees would be more targeted.

The change is small and test-only, and the PR reports the test project passing with no snapshot changes. Approving from my side.

Run 41627256356 completed successfully on current head 8c2550f, so the earlier wait has ended. All current CI checks pass, Codex reports completed with no findings for this head, and the promotion-triggered Claude review explicitly approves. All review surfaces were re-read; there are no inline threads or blocking findings.

Claude's optional regression-sensitivity suggestion retains the disposition recorded above: this PR replaces a constant assertion with actual generation and compilation, delivering #7428; historical mutation testing is follow-up hardening. Checking the whole output compilation is intentional here: it verifies that the generated code compiles with the supplied references. Reference gaps should fail the test and be corrected rather than excluded. Neither suggestion requires another push. Proceeding through the merge gate.